-
Notifications
You must be signed in to change notification settings - Fork 71
feat: srw #1586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main-3.x
Are you sure you want to change the base?
Conversation
b3b7ada to
367647b
Compare
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
docs/using-the-jdbc-driver/using-plugins/UsingTheSimpleReadWriteSplittingPlugin.md
Outdated
Show resolved
Hide resolved
...les/AWSDriverExample/src/main/java/software/amazon/SimpleReadWriteSplittingMySQLExample.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/plugin/AuroraInitialConnectionStrategyPlugin.java
Outdated
Show resolved
Hide resolved
| this.pluginService = pluginService; | ||
| this.properties = properties; | ||
| this.writeEndpoint = writeEndpoint; | ||
| this.readEndpoint = SRW_READ_ENDPOINT.getString(properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that SRW_READ_ENDPOINT is optional parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it so that SRW_READ_ENDPOINT is required
| } | ||
|
|
||
| private boolean isWriteEndpoint(final @NonNull HostSpec hostSpec) { | ||
| return Objects.equals(hostSpec.getHost(), this.writeEndpoint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use getHostAndPort() and verify equalsIgnoreCase()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we expecting that the port is included in the endpoint?
wrapper/src/main/java/software/amazon/jdbc/plugin/srw/SimpleReadWriteSplittingPlugin.java
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/plugin/srw/SimpleReadWriteSplittingPlugin.java
Show resolved
Hide resolved
wrapper/src/test/java/integration/container/tests/SimpleReadWriteSplittingTest.java
Outdated
Show resolved
Hide resolved
63f3490 to
3f4280b
Compare
wrapper/src/main/java/software/amazon/jdbc/plugin/failover/FailoverConnectionPlugin.java
Outdated
Show resolved
Hide resolved
| properties.setProperty("srwReadEndpoint", "test-db.cluster-ro-XYZ.us-east-2.rds.amazonaws.com"); | ||
| ``` | ||
|
|
||
| If you would like to use the Simple Read/Write Splitting Plugin without the failover plugin, make sure you have the `srw` plugin in the `wrapperPlugins` property, and that the failover plugin is not part of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we use a similar blurb to this for the docs for the RW plugin but I can't remember why we mentioned this for this specific combination of plugins. Should we consider removing it? It seems self-explanatory to me.
| |------------------------------|:-------:|:--------:|:----------------------------------------------------------------------------------------------------------------------------------------------|---------------|--------------------------------------------------------------| | ||
| | `srwWriteEndpoint` | String | Yes | The endpoint to connect to when `setReadOnly(false)` is called. | `null` | `<cluster-name>.cluster-<XYZ>.<region>.rds.amazonaws.com` | | ||
| | `srwReadEndpoint` | String | Yes | The endpoint to connect to when `setReadOnly(true)` is called. | `null` | `<cluster-name>.cluster-ro-<XYZ>.<region>.rds.amazonaws.com` | | ||
| | `verifyNewSrwConnections` | Boolean | No | Enables role verification for new connections made by the Simple Read/Write Splitting Plugin. | `true` | `true`, `false` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | `verifyNewSrwConnections` | Boolean | No | Enables role verification for new connections made by the Simple Read/Write Splitting Plugin. | `true` | `true`, `false` | | |
| | `verifyNewSrwConnections` | Boolean | No | Enables writer/reader role verification for new connections made by the Simple Read/Write Splitting Plugin. | `true` | `true`, `false` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between verifyNewSrwConnections and verifyOpenedConnectionType? It sounds like the first enables verification of both readers and writers when setReadOnly is called, but the second enables verification for only "writers" or only "readers"? If the first is set but the second is not, does it verify both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add "More details below" to the descriptions for the bottom 4 parameters?
| ``` | ||
|
|
||
| ## Simple Read/Write Splitting Plugin Parameters | ||
| | Parameter | Value | Required | Description | Default Value | Example Value | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | Parameter | Value | Required | Description | Default Value | Example Value | | |
| | Parameter | Value | Required | Description | Default Value | Example Values | |
|
|
||
| The values of `srwConnectRetryTimeoutMs` and `srwConnectRetryIntervalMs` control the timing and aggressiveness of the plugin's retries. | ||
|
|
||
| Additionally, to consistently ensure the role of connections made with the plugin, the plugin also provides role verification for the initial connection. When connecting with an RDS writer cluster or reader cluster endpoint, the plugin will retry the initial connection up to `srwConnectRetryTimeoutMs` until it has verified connection to the intended role of the endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Additionally, to consistently ensure the role of connections made with the plugin, the plugin also provides role verification for the initial connection. When connecting with an RDS writer cluster or reader cluster endpoint, the plugin will retry the initial connection up to `srwConnectRetryTimeoutMs` until it has verified connection to the intended role of the endpoint. | |
| Additionally, to consistently ensure the role of connections made with the plugin, the plugin also provides role verification for the initial connection. When connecting with an RDS writer cluster or reader cluster endpoint, the plugin will retry the initial connection up to `srwConnectRetryTimeoutMs` until it has verified the intended role of the endpoint. |
|
|
||
| Additionally, to consistently ensure the role of connections made with the plugin, the plugin also provides role verification for the initial connection. When connecting with an RDS writer cluster or reader cluster endpoint, the plugin will retry the initial connection up to `srwConnectRetryTimeoutMs` until it has verified connection to the intended role of the endpoint. | ||
| If it is unable to return a verified initial connection, it will log a message and continue with the normal workflow of the other plugins. | ||
| When connecting with custom endpoints and other non-standard URLs, role verification on the initial connection can also be triggered by providing the expected role through the `verifyOpenedConnectionType` parameter. Set this to `writer` or `reader` accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we consider changing the name from verifyOpenedConnectionType to verifyInitialConnectionType? I think it would be a bit clearer
| Connection writerCandidateConn = this.getVerifiedConnection(props, hostSpec, HostRole.WRITER, connectFunc); | ||
| if (writerCandidateConn == null) { | ||
| // Can't get writer connection. Continue with a normal workflow. | ||
| return connectFunc.call(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we set the initial connection host spec in this case as well or no?
| this.getVerifiedConnection(props, hostSpec, HostRole.READER, connectFunc); | ||
| if (readerCandidateConn == null) { | ||
| // Can't get a reader connection. Continue with a normal workflow. | ||
| return connectFunc.call(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as above
| private final String readEndpoint; | ||
| private HostSpec readEndpointHostSpec; | ||
| private HostSpec writeEndpointHostSpec; | ||
| private final AuroraInitialConnectionStrategyPlugin.VerifyOpenedConnectionType verifyOpenedConnectionType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move VerifyOpenedConnectionType to its own separate class since it is now shared? The VERIFY_OPENED_CONNECTION_TYPE property can be moved there as well
| // No HostSpec provided, still verify role. | ||
| candidateConn = connectFunc.call(); | ||
| } else { | ||
| candidateConn = this.pluginService.connect(hostSpec, props, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if hostSpec is not null and connectFunc is not null, should we use connectFunc instead of pluginService.connect?
| try { | ||
| TimeUnit.MILLISECONDS.sleep(this.connectRetryIntervalMs); | ||
| } catch (InterruptedException ex) { | ||
| // ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // ignore | |
| Thread.currentThread().interrupt() |
Summary
introduces the Simple Read/Write Splitting Plugin (srw)
Description
an alternative to the existing Read/Write Splitting Plugin that connects directly to specified read and write endpoints
Additional Reviewers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.